fix(studio): let a failed DOM edit report that it failed - #3510

Wires a caller-visible outcome onto handleDomTextCommit, handleDomStyleCommit, handleDomEditElementsDelete, and lets useDomEditPositionPatchCommit reject on a paused save queue so the caller's optimistic revert fires. Contract preservation looks solid: runDomEditCommit's resolve-always shape is unchanged; runReportedDomEditCommit layers reporting on top via onSettled, forwarding a caller-supplied callback rather than clobbering it. Human-facing behaviour is asserted preserved (toast fires, DOM reverts). Overall verdict: FINDINGS (P2/nits) — COMMENT pending stamp, no blockers. CI is all green at HEAD 6cbbac02.

Standards checklist

• (a) Types/typecheck: clean per PR; Promise<void> → Promise<unknown> widening propagated to 14 sites; consolidation deferred by note.
• (b) Tests: semantic assertions (toEqual({ ok: false, reason: 'persist-failed' }), toast-fired, DOM-reverted). New useDomEditPositionPatchCommit.test.tsx covers paused-queue, ordinary-fail, success.
• (c) Edge cases: covered for text/style (no-selection, geometry-property, styles-not-editable, not-text-editable, persist-failed). Delete outcome shape is under-covered (see F1).
• (d) Contract invariants: runDomEditCommit resolve-always preserved; onSettled fires exactly once per docstring; runReportedDomEditCommit respects that.
• (e) Telemetry: trackStudioSaveFailure still fires for ordinary position-patch failures; deliberately skipped on paused-queue (banner-only path). No change in classification.
• (f) i18n: no user-facing strings touched.

Editor-UI parity lens (Studio)

• Mid-drag safety: geometry-commit .catch in useDomGeometryCommits still restores optimistic offset/size/rotation and rethrows — now the paused-queue path actually reaches it. Verified.
• Selection state after mutation: delete-failure path preserves selection (unchanged from pre-diff).
• Error-swallowing: the failing test would have caught it — handleDomStyleCommit/handleDomTextCommit reporting is asserted on the persist-failure path with outcome.reason === 'persist-failed', not just presence. handleDomEditElementsDelete outcome asserted only via type, not test.

Findings

P2 — handleDomEditElementsDelete SDK-success path returns undefined, breaking the outcome contract this PR just introduced. packages/studio/src/hooks/useElementLifecycleOps.ts:145 bare-returns return; inside if (allHandled) { ... } while :212 returns { ok: true } as const for the REST-success path and :218 returns domEditCommitDeclined("persist-failed") for the catch. Callers checking outcome.ok === true see undefined for every SDK-cutover delete. Existing tests (useElementLifecycleOps.multiDelete.test.tsx) don't assert the return shape, so this passes CI silently. Suggest return { ok: true } as const; on line 145 and one test that asserts the SDK-success return shape.

P2 — Same bug class the PR fixes still lives in handleDomManualEditsReset. packages/studio/src/hooks/useDomGeometryCommits.ts (the reset handler around the manual-edits-reset callback) does void commitPositionPatchToHtml(...).catch(() => undefined); after already calling clearStudioPathOffset/BoxSize/Rotation on the live element. Under a paused save queue this now rejects (post-PR) but the caller swallows it — the preview stays cleared while nothing reached the file, next reload snaps the manual edits back. Same shape as the position-patch bug the PR describes in its Why. Out of the PR's stated scope but worth a follow-up ticket or a scope-widening line in the body.

Nit — console.error("rotate commit failed", …) / "resize commit failed" in useDomEditOverlayGestures.ts:411,495 now fire for DomEditSaveQueueOpenError too. Before this PR the swallow ate the queue-open error before the geometry commit's .catch ever saw it. Post-PR it propagates through and hits these console.errors, which is a wait condition, not an error. Consider if (!(error instanceof DomEditSaveQueueOpenError)) before logging — otherwise the paused-queue banner event ships a spurious console.error per blocked gesture. Small analytics/log-noise concern only.

Nit — Tagged-outcome docstring on DomEditCommitDeclineReason doesn't mention that persist-failed also covers capture/apply throwing before persist is reached. runReportedDomEditCommit initializes landed = false and only flips true from onSettled(true); if capture()/apply() throws synchronously, the exception propagates and runReportedDomEditCommit rejects rather than returning { ok: false, reason: 'persist-failed' }. Fine — matches runDomEditCommit's pre-existing behaviour — but the "resolves on persist failure by design" note conflates the two paths. Optional tightening.

— Via

🟠 Second-independent read on the delete-outcome contract gap — same P2 landing point as Via's review 5036548919. Two-reviewer convergence on the shape-consistency blocker; three additional differentiated concerns below. HEAD 6cbbac0.

Endorse (co-witness with Via)

Blocker — useElementLifecycleOps.ts:145 bare return; on the SDK-happy path breaks the outcome contract this PR establishes. Verified at HEAD: if (allHandled) short-circuits with showToast(...); return; while :212 returns { ok: true } as const on the REST-success path and :218 returns domEditCommitDeclined("persist-failed") in the catch. Callers checking outcome?.ok === true see undefined on every SDK-cutover delete — indistinguishable from {ok: false, reason: "persist-failed"} at truthy-check and untagged at the type level. Fix: return { ok: true } as const; on :145.

Two more of Via's findings I also traced and read the same way:

• handleDomManualEditsReset residual (useDomGeometryCommits.ts) — same bug-class, void commitPositionPatchToHtml(...).catch(() => undefined); after clearing offset/box/rotation on the live element; post-PR the paused-queue reject becomes a real signal that gets swallowed. Adjacent to this PR's stated scope; worth a follow-up ticket or scope-widen line.
• console.error blast-radius nit at useDomEditOverlayGestures.ts:411,495 — a DomEditSaveQueueOpenError now reaches the log site as a spurious error per blocked gesture. if (!(error instanceof DomEditSaveQueueOpenError)) guard is the right shape.

Additional findings (not in Via's review)

1. No test coverage for handleDomEditElementsDelete's reporting channel. The PR body claims "reports instead of only toasting" for delete, but the new test file adds zero delete-outcome assertions. Given the blocker above, a "reports a successful SDK-path delete" test would have caught the shape drift. Add tests mirroring the text/style set — SDK-success, REST-success, no-project, no-selection, persist-failed for both HTTP-non-ok and the "Nothing to delete" stale-preview throw. Adds ~50 lines but pins the contract on the biggest surface.

2. "Nothing to delete — the preview was out of date" is bucketed as persist-failed. In the REST path, removeData.changed === false throws a plain Error that the catch treats as persist-failed. Semantically it's preview-stale — a programmatic caller retrying on persist-failed will spin, whereas a preview-stale reason signals "refresh first, then retry." Marginal today (no consumers), real once #3511's agent tools consume outcomes.

3. runReportedDomEditCommit infers landed via onSettled flag (domEditCommitRunner.ts:97-99). If a future refactor of runDomEditCommit grows a failure mode that bypasses onSettled (e.g. throws before the try/catch), the wrapper silently reports {ok: false, reason: "persist-failed"} — right shape, wrong reason. Not currently reachable, brittle. Wrapping the inner await runDomEditCommit(...) in try/catch and rethrowing would make the invariant explicit.

What I didn't verify

• Whether any callsite up the chain from useDomGeometryCommits's .catch (which now sees a rethrown DomEditSaveQueueOpenError where it used to see silence) has a further .catch that swallows unknown error classes silently — the new rejection has to terminate somewhere.
• Windows CI job passed (14m7s), but I did not scan the specific test files it hit to confirm the new position-patch test actually ran on Windows — shard names collapsed in gh pr checks.
• Whether #3511's studio_look (the outcome consumer up the stack) actually reads outcome.ok yet — that wiring may live further in the 10-PR stack.

— Review by Rames D Jusso

Resolution for the #3510 findings.

SDK success returns undefined.

Fixed in 2278c11d4. useElementLifecycleOps.ts:138-145 now returns { ok: true }, with the SDK witness at useElementLifecycleOps.multiDelete.test.tsx:92-105.

Manual reset swallows a failed save.

Fixed in 2278c11d4. useDomGeometryCommits.ts:130-154 captures all three geometry states, restores them on rejection, and rethrows through the existing reporting path.

Save-queue waits now log as ordinary errors.

Fixed in 2278c11d4. useDomEditOverlayGestures.ts:61-65 excludes DomEditSaveQueueOpenError; anchoredResizeCommitFeedsOffset.test.ts:230-246 proves the wait is quiet while real failures still log.

Delete outcomes are untested, and stale preview is reported as persist-failed.

Fixed in 2278c11d4 and 2a0a034dd. Delete coverage is at useElementLifecycleOps.multiDelete.test.tsx:80-148; useElementLifecycleOps.ts:172-179 now returns preview-stale directly and uses the corrected toast copy.

The landed inference is brittle if a future failure bypasses onSettled.

This does not reproduce in the current contract. domEditCommitRunner.ts:40-52 runs capture and apply before the persist try, so those bugs reject through the wrapper. The only handled failure is persist, and both persist outcomes call onSettled. Lines 62-65 now state that boundary explicitly.

🟢 #3510 R2 verdict — cleared by shape at 2a0a034d. Every R1 concern (mine + Via's) verified fixed at the file:line Miguel cited; no new hazards introduced.

R1 concerns — verified fixed by shape:

• P2 blocker (bare return; on SDK-happy delete path): PASS. useElementLifecycleOps.ts:145 now returns { ok: true } as const;. Contract shape now matches :213 (REST-success) and :219 (catch → persist-failed).
• Delete-outcome test coverage: PASS. useElementLifecycleOps.multiDelete.test.tsx now pins the full matrix — SDK-success (:92-105, "reports a successful SDK delete as landed"), REST-success (:89), no-project/no-selection (:108-122), persist-failed on HTTP 500 (:124-132), preview-stale (:134-149). Would catch the blocker on regression.
• "Nothing to delete" → persist-failed misclassification: PASS. useElementLifecycleOps.ts:172-179 now returns domEditCommitDeclined("preview-stale") directly instead of throwing into the catch. preview-stale added to the union at domEditCommitRunner.ts:75. A programmatic caller retrying on persist-failed no longer spins on a stale preview.
• runReportedDomEditCommit landed-via-onSettled brittleness: CLEARED by mechanism, not paved-over. Miguel's rebuttal holds — capture()/apply() run before the try at domEditCommitRunner.ts:40-42, so their throws propagate through await runDomEditCommit(...) in runReportedDomEditCommit, which means the {ok: false, reason: "persist-failed"} return is genuinely gated on persist-only. The :62-65 docstring now states this: "Capture and apply bugs still reject." Right-shape-right-reason invariant is now explicit. Not a carry-forward ticket.
• Via's residual — handleDomManualEditsReset rollback: PASS. useDomGeometryCommits.ts:130-154 captures beforeOffset/beforeSize/beforeRotation up front, restores all three in .catch, then rethrows. Test at useDomGeometryCommits.test.tsx:55 asserts rollback on rejection. Return type widened void → Promise<void> at useDomEditWiring.ts:110 and useGsapSelectionHandlers.ts:114; the sole invocation at useGsapSelectionHandlers.ts:233 does void handleDomManualEditsReset(...).catch(() => undefined) — intentional UI-boundary swallow, documented inline, safe because rollback lives inside and telemetry lives on the position commit.
• Via's residual — save-queue console.error blast: PASS. New helper logGestureCommitFailure at useDomEditOverlayGestures.ts:63-66 skips DomEditSaveQueueOpenError; both call sites (rotate :418, resize :502) use it. Test at anchoredResizeCommitFeedsOffset.test.ts:229-247 proves both directions — paused-queue is quiet, ordinary failures still log with ("resize commit failed", failure).

What I didn't verify:

• CI is not yet fully green at HEAD — Analyze/Build/Lint/Format/Typecheck/CodeQL/SDK unit+contract+smoke/Preflight/Studio: load smoke/Perf: * all pass, but regression-shards (shards 2/3/5/6/7/8), Tests on windows-latest, CLI smoke (required), Test, Smoke: global install are still pending. One top-level regression wrapper shows fail at 4s — reads as a meta-check reflecting the still-pending shards rather than a real regression. mergeStateStatus=BLOCKED, mergeable=MERGEABLE.
• Whether any additional consumer of the widened Promise<unknown> outcome (14 hand-copied call-prop sites Miguel flagged as consolidation-worthy) actually reads outcome.ok yet — that wiring lives up the stack in #3511+ and is out of R2 scope.

Peer state: Via's R2 review 5036548919 (COMMENTED, 2026-08-27 01:46) was posted at pre-fix SHA 6cbbac0 — she flagged the SAME P2 blocker independently and raised the manual-reset + save-queue log concerns. Miguel's R2 push at 2a0a034d addresses her three findings AND my three differentiated findings in a single pass. Via has not yet re-reviewed at R2 HEAD.

Stack hygiene note (not a #3510 blocker): #3511's baseRefOid still points at 6cbbac02 (R1 HEAD of #3510) rather than R2 HEAD 2a0a034d, so #3511 hasn't been restacked onto Miguel's fixes yet. When it does restack, it'll pick up the new outcome-shape returns (delete-outcome + preview-stale union member) — worth remembering for the #3511 gate. #3511 + #3514 currently both CLEAN.

— Review by Rames D Jusso

R2 delta re-verify (HEAD 2a0a034d)

Prior review head was 6cbbac02. All four claims verified by shape against the diff.

R1 findings — delta

• P2 #1 — outcome-shape inconsistency in handleDomEditElementsDelete: fixed.

  • packages/studio/src/hooks/useElementLifecycleOps.ts:145 — SDK-cutover success path now return { ok: true } as const instead of bare return;.
  • useElementLifecycleOps.ts:175 — stale-preview path now showToast(...); return domEditCommitDeclined("preview-stale") in place of the throw. Human-visible toast preserved; the outer catch is no longer the messenger for this case, which is what let the pointer close.
  • useElementLifecycleOps.ts:210 / :216 — HTTP success returns { ok: true } as const; catch returns domEditCommitDeclined("persist-failed"). New tests useElementLifecycleOps.multiDelete.test.tsx:1051-1091 assert the exact tagged shapes across SDK-commit / no-project / no-selection / persist-fail / stale-preview.

• P2 #2 — same swallow-bug class in useDomGeometryCommits.ts handleDomManualEditsReset: fixed.

  • packages/studio/src/hooks/useDomGeometryCommits.ts:130-152 — signature now returns Promise<void>; the .catch restores captured beforeOffset / beforeSize / beforeRotation via the restoreStudio* helpers and rethrows.
  • Interface propagation checked: useDomEditWiring.ts:110 and useGsapSelectionHandlers.ts:114 both updated to Promise<void>. The only actual invocation site — useGsapSelectionHandlers.ts:233 — uses void handleDomManualEditsReset(sel).catch(() => undefined) as the fire-and-forget UI boundary, with a comment explaining that position-commit already owns the user report and the reset owns rollback.
  • useDomGeometryCommits.test.tsx:55 extends the existing rollback assertion to cover the reset path.

• Nit — console.error spam on DomEditSaveQueueOpenError: fixed.

  • packages/studio/src/hooks/useDomEditPositionPatchCommit.ts:38-46 — queue-error branch now rethrows without toasting or calling trackStudioSaveFailure; the toast + telemetry stay scoped to the non-queue branch.
  • useDomEditOverlayGestures.ts:61-66 — new logGestureCommitFailure helper filters DomEditSaveQueueOpenError from both the rotate commit failed and resize commit failed paths (:415 and :499). Tests anchoredResizeCommitFeedsOffset.test.ts:113-130 assert both: no spam on paused queue, ordinary failures still log.

Adjacent-defect check (6 axes at the fix boundary)

• Types: the outcome plumbing widened onSetStyle / onCommit return types to void | Promise<unknown> across propertyPanelCommitField.tsx:24, the flat panel propertyPanelFlat* files, useInspectorGestureTransaction.ts:15,125, and DomEditSelectionChrome.tsx:126. This is a widening — existing Promise<void> callers still satisfy it. No caller I could find destructures the outcome.
• Callers: handleDomManualEditsReset grep at HEAD returns one true invocation site (already audited above); the other hits are interface plumbing only.
• Tests: outcomes are asserted by structural equality (toEqual({ ok: false, reason: "..." })), so a future silent mutation of the reason string will fail the test rather than pass on a truthy shape.
• Error paths: capture / apply throws still propagate as rejections from runReportedDomEditCommit — documented in the DomEditCommitDeclineReason doc-comment (domEditCommitRunner.ts:39-46). The docstring makes the two exit shapes explicit (reject vs. {ok:false}), which is what a caller mixing .then / .catch will need to reason about.
• Concurrency: the position-patch rethrow now feeds the caller's optimistic revert, closing the drag-persist race that was the reason this bug had a user-visible tail. useDomEditPositionPatchCommit.test.tsx:22-58 covers the three shapes (queue-paused → reject-no-toast, ordinary-failure → reject-with-toast, success → resolve).
• Telemetry: trackStudioSaveFailure is still called on ordinary save failures and correctly skipped on DomEditSaveQueueOpenError. No path lost a report.

Adversarial fingerprint

For handleDomStyleCommit, a manual-geometry property like left now returns { ok: false, reason: "geometry-property" } (useDomEditTextCommits.ts:190). A caller that only checks outcome.ok will treat this as "did not land" — but the style path deliberately refuses these because manual-geometry lives on a different code path, so this is intended and matches the docstring. Not a defect.

CI

gh pr checks 3510: 51 pass, 2 skipping, 5 pending, 1 fail. The one fail is the regression aggregation from run 33034712644, red because its shards were cancelled != success; the shards on the newer run 33034885989 are still pending and several have already gone green. Re-verify once shards settle.

Verdict

R1 findings resolved; ready for approval by another reviewer.

— Via

APPROVED at 2a0a034dd5914a7e2aca732acd3f83d0825cbdfb.

Read the diff independently rather than stamping on the green R2s. The outcome contract holds: onSettled fires exactly once on both branches of runDomEditCommit (domEditCommitRunner.ts:46,52) and both precede the resync at :55, so landed is always set by the time runReportedDomEditCommit resolves. Every top-level path in all three producing handlers now returns an outcome — the :145 bare return that started this is gone.

One non-blocking finding: the contract is enforced nowhere in the type system.

DomEditCommitOutcome appears exactly three times in this 1202-line diff — the type declaration, domEditCommitDeclined's return type, and runReportedDomEditCommit's. It is never a handler's return type. All three producers (handleDomStyleCommit, handleDomTextCommit, handleDomEditElementsDelete) are bare async (…) => {…} relying on inference.

That means a future bare return; in any of them silently widens the inferred type to … | undefined rather than failing the build — which is exactly the bug this PR exists to remove, reintroduced with no compiler complaint. The delete test matrix pins today's paths; it can't pin a path that doesn't exist yet. An explicit : Promise<DomEditCommitOutcome> on each of the three is a one-line-each durable guard, and worth having before the write tools downstream start depending on the shape.

Two things I chased and cleared, so nobody reopens them:

• capture bailing on a missing element does not make {ok: true} a lie. In handleDomStyleCommit both capture and apply early-return when the element isn't in the preview DOM, but persist calls persistDomEditOperations(domEditSelection, operations, …) — payload derived from the selection descriptor, not from editedElement. The file genuinely changes; only the optimistic in-preview paint is skipped. Pre-existing preview/file divergence, outside this fix boundary.
• The bare return;s remaining inside the handlers are in the capture/apply/revert closures, which are correctly void. Not stragglers.

autoMergeRequest=null, so this stamp is a stamp and the merge stays yours. Not gating on the windows-latest red — CI isn't mine to hold a stamp on — but it is real and yours to clear before merge.

— Rames

APPROVED at 69c4402f74b38954a71e36e7180ed5a92615e531. Finding fixed, verified at source.

All three producers now carry the annotation — handleDomStyleCommit and handleDomTextCommit (useDomEditTextCommits.ts:189,273) and handleDomEditElementsDelete (useElementLifecycleOps.ts:88) — imported type-only, so nothing is added at runtime. The delta is 5 added and 4 removed across exactly the two files that hold them, with no drive-by changes riding along.

I checked the two things that could have made this cosmetic rather than real:

• The annotation actually bites. packages/studio/tsconfig.json sets "strict": true, so strictNullChecks is on and undefined is not assignable to DomEditCommitOutcome. A bare return; is now a type error, and so is a path that falls off the end — TS reports the missing ending return precisely because the declared type excludes undefined. Without strict mode this would have compiled and the guard would have been inert, which is the version of this fix that looks identical in a diff.
• No consumer erases it. Nothing in useDomEditWiring.ts or propertyPanelTypes.ts re-declares these three with a Promise<void> signature that would flatten the contract back at the boundary.

So the type system is now the witness, and it will stay the witness without anyone maintaining a test for it. That's the durable version.

— Rames

R3 delta re-verify (HEAD 69c4402f)

Prior verified HEAD was 2a0a034d. Miguel's follow-up addresses Rames's R2 non-blocking finding — that the outcome contract was runtime-only and would not stop a future bare return; from re-introducing the silent-swallow bug.

1. Delta scope

gh api compare 2a0a034d...69c4402f returns one commit ("fix(studio): enforce DOM edit outcome types") touching two files:

• packages/studio/src/hooks/useDomEditTextCommits.ts (+3, −2)
• packages/studio/src/hooks/useElementLifecycleOps.ts (+2, −2)

No other files, no drive-by edits, no test changes in the delta. The 15 runtime-shape tests Rames and I both leaned on for R1/R2 are the pre-existing suite; the delta pulls its weight from the type system, not from new tests.

2. Type-witness claim — substantiated

Three producers now carry the annotation:

• handleDomStyleCommit — useDomEditTextCommits.ts:189 → async (property, value): Promise<DomEditCommitOutcome> => …
• handleDomTextCommit — useDomEditTextCommits.ts:273 → same shape
• handleDomEditElementsDelete — useElementLifecycleOps.ts:88 → same shape

Imports are type-only in both files, so nothing is added at runtime.

The annotation bites — I verified two independent gates:

• Strictness. packages/studio/tsconfig.json at 69c4402f sets "strict": true, which turns on strictNullChecks. undefined is not assignable to { ok: true } | { ok: false; reason: DomEditCommitDeclineReason }, so a future bare return; is a compile error and so is a fall-through path — TS reports the missing return precisely because the declared type excludes undefined. Without strict, the annotation would be inert and the fix would look identical in a diff.
• No consumer flattens it back. Grepping the delta and its neighbouring consumers, no Promise<void> re-declaration re-widens the contract at the boundary. The wider Promise<unknown> types on onStyleCommit etc. in R2 accept the narrower Promise<DomEditCommitOutcome> — they don't erase it at the producer.

Typecheck passed on this HEAD (Actions runs/33036722304/job/98400920649), which is the direct proof the annotation compiles.

3. Test discipline

Delta adds no tests. The pre-existing R2 tests (useDomEditTextCommits.test.tsx, useElementLifecycleOps.multiDelete.test.tsx, useDomEditPositionPatchCommit.test.tsx, anchoredResizeCommitFeedsOffset.test.ts) all assert runtime shape — expect(outcome).toEqual({ ok: false, reason: "persist-failed" }) and friends — not type existence. Runtime discipline stayed; this delta lets the type system replace the "will anyone remember to test the new path" fragility with a compiler check.

4. Adversarial — throws still bypass the type witness (accepted)

A Promise<T> type constrains the resolved value only; a rejection carries any thrown error regardless of T. So the annotation catches "handler falls through and resolves undefined" but not "handler throws before reaching a return".

Walking each producer to confirm the surviving throw paths are correctly routed to rejection rather than silent completion:

• handleDomStyleCommit / handleDomTextCommit — runReportedDomEditCommit awaits runDomEditCommit, which by contract calls onSettled(ok) on both persist paths (domEditCommitRunner.ts:46,52) and swallows persist failures. A capture/apply-shaped defect rejects — which is the intended signal (defect, not handled failure), distinct from { ok: false }, and the caller can distinguish them by .catch vs. resolved outcome.
• handleDomEditElementsDelete — wraps its main path in try/catch and returns domEditCommitDeclined("persist-failed") on error, so even a throw resolves as a proper outcome; the type witness is genuinely total here.

Nothing to raise — this is the expected shape.

5. CI

Typecheck, Build, Lint, Format, Preflight, CLI smoke (required), Producer unit + integration, Studio load smoke + timeline viewport gate, Test: skills, Test: runtime contract, SDK unit+contract+smoke — all green at 69c4402f. Perf shards and one Analyze pass still in-progress at review time; no non-success conclusions anywhere on the run. windows-latest — the red Rames flagged at R2 — is not present at this HEAD.

Verdict

Fix substantiated at source. Type-witness is real, not cosmetic. Non-approving per protocol; deferring stamp to the human path.

— Via